-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HPCC-29332 Add opentelemetry based tracing #17538
Conversation
- Adds opentel cpp lib + otlp + otlp-http Signed-off-by: Rodrigo Pastrana <[email protected]>
https://track.hpccsystems.com/browse/HPCC-29332 |
@GordonSmith I will add cmake option to suppress this feature if it makes sense in the future, right now it seems open telemetry will be part of the 9.4+ platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpastrana you should include this change in your PR for Open Telemetry Support as there is no benefit to having it in master without it (and there is a slight downside to including it when not needed)?
- WIP Signed-off-by: Rodrigo Pastrana <[email protected]>
Thanks @GordonSmith, that makes sense, Gavin's Jira listed this change as a stand-alone subtask but I can merge together. |
That change to vcpkg.json.in looks fine - this is an "always on" thing? |
It's likely to be an always-on feature once implemented. What are the vcpkg side effects of making it optional based on a cmake flag? |
You wouldn't make it optional if its always needed - so what you have is good. |
Clearly, and that's why it's currently not optional, the flag would be to suppress it until needed. |
…into HPCC-29332-OpenTelCpp
- WIP - Adds system/tracing/tracemanager - Adds instrumented server/client samples Signed-off-by: Rodrigo Pastrana <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only looked at the cmake stuff.
system/tracing/CMakeLists.txt
Outdated
@@ -0,0 +1,73 @@ | |||
################################################################################ | |||
# HPCC SYSTEMS software Copyright (C) 2012 HPCC Systems®. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Year is wrong (do we need this block anyway?)
system/tracing/CMakeLists.txt
Outdated
|
||
if (opentelemetry-cpp_FOUND) | ||
MESSAGE(STATUS "Found Opentelemetry") | ||
MESSAGE("^^^^^^^^^${OPENTELEMETRY_CPP_INCLUDE_DIRS}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these needed - it will fail above with the REQUIRED?
system/tracing/CMakeLists.txt
Outdated
jlib | ||
) | ||
else() | ||
MESSAGE(STATUS "Opentelemetry libs/headers not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these needed - it will fail above with the REQUIRED?
system/tracing/CMakeLists.txt
Outdated
${CMAKE_BINARY_DIR}/oss | ||
${HPCC_SOURCE_DIR}/system/tracing | ||
) | ||
#SET ( SRCS tracemanager.hpp opentel/instrumented/httpserver.cpp opentel/instrumented/httpclient.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented code.
system/tracing/CMakeLists.txt
Outdated
opentelemetry-cpp::ostream_span_exporter | ||
#opentelemetry-cpp::jaeger_trace_exporter | ||
#opentelemetry-cpp::otlp_grpc_exporter | ||
#opentelemetry-cpp::otlp_grpc_log_exporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented code
|
||
#find_package(opentelemetry-cpp CONFIG REQUIRED) | ||
|
||
if (opentelemetry-cpp_FOUND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file won't be included if its not found?
#find_package(opentelemetry-cpp CONFIG REQUIRED) | ||
|
||
if (opentelemetry-cpp_FOUND) | ||
MESSAGE(STATUS "Found Opentelemetry") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
opentelemetry-cpp::ostream_span_exporter | ||
#opentelemetry-cpp::jaeger_trace_exporter | ||
#opentelemetry-cpp::otlp_grpc_exporter | ||
#opentelemetry-cpp::otlp_grpc_log_exporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented code
opentelemetry-cpp::http_client_curl | ||
jlib) | ||
else() | ||
MESSAGE(STATUS "Opentelemetry libs/headers not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
jlib) | ||
else() | ||
MESSAGE(STATUS "Opentelemetry libs/headers not found") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be an INSTALL?
https://track.hpccsystems.com/browse/HPCC-29332 |
1 similar comment
https://track.hpccsystems.com/browse/HPCC-29332 |
@ghalliday this PR is nowhere near ready for review but as discussed, wanted to share with you to review at a very high level while I'm out. We can discuss when I return |
08b8eff
to
0fee5c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments on my first pass are a bit random, but the general impression is:
- The open telemetry code should be encapsulated within jtrace, and ideally would not leak outside it. (That may not be completely possible, but it is a good initial ideal.)
- The jtrace code should have classes which map to the different concepts that we want to support and can be used to manage them. That was not clear when I looked at it.
- Generally we use interfaces rather than template classes - see a more detailed comment above.
I will reread it through again before you get back - I expect some of it will become clearer.
system/jlib/jtrace.hpp
Outdated
@@ -18,21 +18,197 @@ | |||
#ifndef JTRACE_HPP | |||
#define JTRACE_HPP | |||
|
|||
#undef UNIMPLEMENTED //opentelemetry defines UNIMPLEMENTED | |||
#include "opentelemetry/exporters/ostream/span_exporter_factory.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid these includes in the header file if at all possible... Otherwise everything in the system is suddenly going to be including many more header files.
system/jlib/jtrace.hpp
Outdated
namespace opentel_trace = opentelemetry::trace; | ||
|
||
//#include "jexcept.hpp" //re-define UNIMPLEMENTED | ||
#define UNIMPLEMENTED throw makeStringExceptionV(-1, "UNIMPLEMENTED feature at %s(%d)", sanitizeSourceFile(__FILE__), __LINE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are cleaner ways of achieving this
#define OLD_X X
#undef X
#include....
#undef X
#define X OLD_X
But it also shouldn't be in the header file unless there is no other solution.
system/jlib/jtrace.hpp
Outdated
// virtual Keys(nostd::function_ref<bool(nostd::string_view)> /* callback */) | ||
// list of all the keys in the carrier. | ||
// By default, it returns true without invoking callback */ | ||
template <typename T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong being a template class - it suggests a design decision hasn't been made, and requires everything is in the header - which will cause compile sloath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This largely as-is example from OpenTel doesn't look wrong to me given what they were hoping to accomplish (get,set,keys for various representations of "httpheaders"), but we can provide certainly go a different way (do I recall you mentioning a lambda function interface )?
|
||
//Declare the span, provide appropriate attributes, and options | ||
//Trace ID generated if no parent context is provided | ||
auto processingRequestSpan = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect much of this code to be encapsulated inside jtrace. That may not be possible, but having open telemetry code that manages spans scattered through the code suggests poor encapsulation.
@@ -450,6 +483,9 @@ int CEspHttpServer::processRequest() | |||
return 0; | |||
} | |||
|
|||
//need to ensure that the span is ended when out of scope Owend<ISpan> processingRequestSpan? | |||
processingRequestSpan->End(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be managed by a local class object - otherwise any exceptions thrown will not call this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
system/jlib/jtrace.cpp
Outdated
//Target propogator? http? grpc? binary? custom? | ||
//HPCC component tracing switches? | ||
Owned<const IPropertyTree> traceConfig; | ||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure you need the try block. If this throws an exception it suggests you are calling it at the wrong point.
system/jlib/jtrace.hpp
Outdated
static constexpr const char *kCallerIdHTTPHeader = "HPCC-Caller-Id"; | ||
} | ||
|
||
// github copilot generated comment: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't really make it any clearer!
system/jlib/jtrace.hpp
Outdated
|
||
TraceManager(const std::string & moduleName) | ||
{ | ||
TraceManager(moduleName.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost certainly going to lead to traceName pointing at a name that has been fred.
system/jlib/jtrace.hpp
Outdated
static opentelemetry::nostd::shared_ptr<opentelemetry::trace::Tracer> getTracer(std::string moduleName); | ||
|
||
//Extracts parent contex from the carrier's headers and returns it as callerSpanId | ||
static void getCallerSpanId(context::propagation::TextMapCarrier &carrier, std::string & callerSpanId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect these should be member functions of another class, rather than static functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss offline to clearly define "another class"
system/jlib/jtrace.hpp
Outdated
// HPCCHttpTextMapCarrier<IProperties> , HttpTextMapCarrier<http_client::Headers>, etc | ||
// The carrier must implement the TextMapCarrier interface | ||
// and it will determine the header which contains the parent span | ||
template<class CARRIER> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again the use of templates suggests a lack of design. The HPCC codebase tends to use interfaces rather than templates classes.
templates are generally reserved for
- simple integer types
- generic general purpose container classes (e.g. hash table)
- rarely implementing classes on top of other base classes.
I think the general concept is good. But I would vastly simplify the abstraction, especially in the beginning. Then as I added features I could be forced to expose a little more complexity. But basically my first pass would to implement something like:
|
esp/platform/espp.cpp
Outdated
@@ -426,6 +427,11 @@ int init_main(int argc, const char* argv[]) | |||
//save off generated config to register with container. Legacy can always reference the config file, application based ESP needs generated config saved off | |||
Owned<IPropertyTree> appConfig; | |||
|
|||
TraceManager traceManager("esp"); | |||
auto espTracer = traceManager.getTracer();//All instances of this esp process can share the same tracer same as TraceManager::getTracer("esp") | |||
auto rootEspProcspan = espTracer->StartSpan(__func__); //Dummy top level span for the esp process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dummy span really needed? If so, at least for now, I would completely hide it from a process that handles requests. Even for job type processes, like say eclagent, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required, the component owner will define the appropriate spans, this illustrates a top level span for the current function
Spans for queued workunits and agent jobs would be a different discussion. |
Great comments and I'm in agreement. |
@ghalliday thanks for all the insightful comments, I prob should have been more explicit that this PR is nowhere near ready and only provided to invoke high level conversation, as you suggested we can discuss actual requirements and high level structure offline. (please don't expect replies to all your current comments) |
- Removes system/tracing/tracemanager Signed-off-by: Rodrigo Pastrana <[email protected]>
0fee5c2
to
eeaf913
Compare
Signed-off-by: Rodrigo Pastrana <[email protected]>
Signed-off-by: Rodrigo Pastrana <[email protected]>
Signed-off-by: Rodrigo Pastrana <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpastrana from an initial scan that looks much closer to what I would hope for. More of the implementations can be moved from the header file to the cpp file. I think there is a good chance you can remove all references to open telemetry in the header if you do that.
I will go back and look at it much more closely.
system/jlib/jtrace.hpp
Outdated
virtual void activate() = 0; | ||
}; | ||
|
||
class CSpan : public CInterfaceOf<ISpan> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation can also move into the cpp file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpastrana some more detailed comments. A brief summary form memory:
- Perform the traceManager initialisation once when the config is processed.
- I don't think we need IHPCCTracer as well as ITraceManager
- Simplify the interface a bit by removing a few functions, and not passing attributes when a span is created.
- activate() not needed
- move some more code into the cpp file from the header.
@@ -184,6 +184,26 @@ void checkSetCORSAllowOrigin(EspHttpBinding *binding, CHttpRequest *req, CHttpRe | |||
|
|||
int CEspHttpServer::processRequest() | |||
{ | |||
Owned<IHPCCTracer> tracer = queryTraceManager()->initTracing("esphttpserver"); //Initialize the trace manager, and ESP trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect the trace manager to only be intialised at process startup, rather than per request. Ideally in loadConfiguration so that it was common to all components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure you need the tracer interface - is there a situation where you need more than one per process?
Owned<IHPCCTracer> tracer = queryTraceManager()->initTracing("esphttpserver"); //Initialize the trace manager, and ESP trace | ||
|
||
//This is the span that will be used to track the processing of the request | ||
Owned<IProperties> reqProcessSpanAttributes = createProperties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The attributes can be modified after the span creation, so it may be simpler (and more flexible/efficient) to not pass the spanAttributes to the span creation, and use span->setProp() afterwards.
reqProcessSpanAttributes->setProp("http.url", m_request->queryPath()); | ||
reqProcessSpanAttributes->setProp("http.host", m_request->queryHost()); | ||
reqProcessSpanAttributes->setProp("http.request_content_length", m_request->getContentLength()); | ||
reqProcessSpanAttributes->setProp("http.request_query_string", m_request->queryParamStr()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this is an example, I suspect we do not want to do this because it will contain PII.
//This is the span that will be used to track the processing of the request | ||
Owned<IProperties> reqProcessSpanAttributes = createProperties(); | ||
reqProcessSpanAttributes->setProp("http.request_port", "8010"); | ||
reqProcessSpanAttributes->setProp("app.name", "esp"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good as an example, but we should only add attributes that are actually useful.
Owned<ISpan> reqProcessSpan = tracer->createTransactionSpan("ProcessingHTTPRequest", mockHTTPHeaders, reqProcessSpanAttributes); | ||
|
||
//ideally the span should be activated when created | ||
reqProcessSpan->activate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Any reason why it cannot be activated?
system/jlib/jtrace.hpp
Outdated
if (!span) | ||
return false; | ||
|
||
if (!span->IsRecording()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this check for? I think recording means something specific for spans, it is not required for a valid span id.
system/jlib/jtrace.cpp
Outdated
} | ||
} | ||
|
||
void CTransactionSpan::setAttriburesFromHTTPHeaders(StringArray & httpHeaders) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: attributes
same for the function below.
hpccCallerId.set(httpHeaders.item(currentHeaderIndex+1)); | ||
continue; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this function and the next to do the same thing. This doesn't contain the latter code.
For simplicitly I would create an IProperties from the array and then call the other function. If it is time critical then investigate some more later.
system/jlib/jtrace.cpp
Outdated
|
||
void CSpan::activate() | ||
{ | ||
auto provider = opentelemetry::trace::Provider::GetTracerProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to use this functionality. Because we can have multiple active spans at the same time we will need to separately track the active span.
system/jlib/jtrace.cpp
Outdated
auto tracer = provider->GetTracer(tracerName.get()); | ||
options.kind = spanKind; | ||
name.set(spanName); | ||
span = tracer->StartSpan(spanName, {}, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this StartSpan should be done a bit differently, but it will become clearer as the structure settles down.
…into HPCC-29332-OpenTelCpp
- Removes sample usage to avoid code review complications - Adds realistic sample in ESP processrequest - Initialize OpenTel at loadConfig time - Migrate OT imports to implementation - Eliminate HPCCTracer interface - Add appropriate interface methods - Does not yet use LogTrace Signed-off-by: Rodrigo Pastrana <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpastrana various comments. We can walk through them later.
Also you seem to have merged master into this branch - that will cause problems later, you need to rebase onto master instead.
system/jlib/jptree.cpp
Outdated
|
||
DBGLOG("Initializing OpenTel based tracing for component: '%s'", componentTag); | ||
//Owned<CTraceManager> tracer = | ||
queryTraceManager(componentTag); //inits opentel singleton internals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
initTraceManager(componentTag, componentConfiguration);
or similar.
system/jlib/jtrace.cpp
Outdated
{ | ||
opentelemetry::trace::StartSpanOptions options; | ||
options.kind = opentelemetry::trace::SpanKind::kClient; | ||
CSpan(&options, spanName, tracerName_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there are reason to prefer this syntax? Nothing else that I know of is coded this way. I don't think it does what you expect, I think this creates a temporary object within the function.
system/jlib/jtrace.cpp
Outdated
} | ||
|
||
CSpan::CSpan(opentelemetry::trace::SpanKind spanKind, const char * spanName, const char * tracerName_, const IProperties * spanAttributes) | ||
void CSpan::setOTTraceID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trivial: structure of the c++ file - should have the CSpan functions together, and before the classes that are derived from them.
system/jlib/jtrace.hpp
Outdated
|
||
virtual void activate() = 0; | ||
class CNoOpSpan : public CInterfaceOf<ISpan> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is best to avoid implementations of interfaces in header files. It can lead the c++ compile to assume this is the most common implementation and special case checks for it. That tends to bloat the code, and since we're using dlls it almost never matches.
system/jlib/jtrace.cpp
Outdated
{ | ||
Owned<IProperties> httpHeaderProps = createProperties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: This could be extracted to a general purpose function that could live in jprop.hpp:
void extractHeaders(IProperties * target, const StringArray & httpHeaders);
|
||
//This span tracks processing of httprequest | ||
//Owned<ISpan> reqProcessSpan = queryTraceManager("esp")->createTransactionSpan("ProcessingHTTPRequest", mockHTTPHeaders); | ||
Owned<ISpan> reqProcessSpan = queryTraceManager("esp")->createTransactionSpan("ProcessingHTTPRequest", mockHTTPHeadersSA); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should not be a name passed to the trace manager (see comment later on).
system/jlib/jtrace.hpp
Outdated
|
||
virtual void activate() = 0; | ||
class CNoOpSpan : public CInterfaceOf<ISpan> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this null class, or would the default implementation do? Unless it is needed I would not bother specialising.
system/jlib/jtrace.hpp
Outdated
nostd::shared_ptr<opentelemetry::trace::Span> span; | ||
}; | ||
|
||
class CTransactionSpan : public CSpan | ||
{ | ||
private: | ||
opentelemetry::v1::trace::SpanContext parentContext = opentelemetry::trace::SpanContext::GetInvalid(); | ||
void setAttriburesFromHTTPHeaders(const IProperties * httpHeaders); | ||
void setAttriburesFromHTTPHeaders(StringArray & httpHeaders); | ||
void setAttributesFromHTTPHeaders(const IProperties * httpHeaders, opentelemetry::trace::StartSpanOptions * options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the options parameter. Possibly ok if this was in the c++ file, not if it was in the header.
system/jlib/jtrace.hpp
Outdated
class CClientSpan : public CSpan | ||
{ | ||
public: | ||
CClientSpan(const char * spanName, const char * tracerName_, const IProperties * spanAttributes); | ||
CClientSpan(const char * spanName, const char * tracerName_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructor for a client span should almost certainly have a ISpan/CSpan as a parent parameter.
system/jlib/jtrace.hpp
Outdated
}; | ||
virtual ISpan * createTransactionSpan(const char * name, StringArray & httpHeaders) = 0; | ||
virtual ISpan * createTransactionSpan(const char * name, const IProperties * httpHeaders) = 0; | ||
virtual ISpan * createClientSpan(const char * name) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably be members of a ISpan rather than a trace manager. They can stay here for the moment, but they should primarily(/exclusively?) be created as a member of an ISpan. They should also have a parent span parameter.
- Provides simple clear interface definition - Provides traceman init and query - Migrate ALL OT imports to implementation - Add appropriate interface methods - Provides manual parent span injection - Does not yet use LogTrace - Does not yet defint helm/k8s schema Signed-off-by: Rodrigo Pastrana <[email protected]>
- Updates interface - Forces specific Server->client/internal span relationship - Starts OTLP support - Attempts to remove all getcurrent references - Does not yet use LogTrace - Does not yet defint helm/k8s schema Signed-off-by: Rodrigo Pastrana <[email protected]>
57d119c
to
3a71c97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpastrana a couple of comments, but looks a good base for discussion.
system/jlib/jtrace.hpp
Outdated
|
||
interface ITraceManager : extends IInterface | ||
{ | ||
virtual ISpan * createServerSpan(const char * name, StringArray & httpHeaders, ISpan * parentSpan) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do server spans have parent spans?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the parent would typically be assigned from caller's context (http headers in this case)
system/jlib/jtrace.hpp
Outdated
virtual void setSpanAttribute(const char * key, const char * val) = 0; | ||
virtual void setSpanAttributes(const IProperties * attributes) = 0; | ||
virtual void addSpanEvent(const char * eventName) = 0; | ||
virtual void setActive() = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated not exposing this because it is very openTel specific, but ultimately decided the caller should have the ability to set a particular trace as the active (parent span for all new spans). If we don't expose this, or similar functionality the caller will be responsible for tracking and explicitly setting the parent span which is sensible I suppose
system/jlib/jtrace.hpp
Outdated
virtual const char * queryHPCCCallerID() = 0; | ||
virtual const char * querySpanName() = 0; | ||
virtual const char * queryTraceID() = 0; | ||
virtual const char * queryTraceFlags() = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear what this function would return, but fine if it has a purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The traceflags are def openTelemetry specific, and I could see an argument to keep the interface agnostic...
They, along with the traced and span id make up the span context which is used to create and inject parentContext
2af10cb
to
3738790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpastrana a few minor comments. I think now would be a good time to squash all the commits, close this PR and open a new one with the branch.
@@ -184,6 +184,48 @@ void checkSetCORSAllowOrigin(EspHttpBinding *binding, CHttpRequest *req, CHttpRe | |||
|
|||
int CEspHttpServer::processRequest() | |||
{ | |||
//Mock http headers from request | |||
Owned<IProperties> mockHTTPHeaders = createProperties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should be moved to some unit tests - probably in jlibtests.cpp. That can then be expanded with other unit tests. E.g. creating server spans with and without incoming traceids, invalid trace ids etc. and check that they behave correctly.
Then can extend to check that client and internal spans can be created on parallel threads and that it doesn't cause problems, and gather overhead for creating the spans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree on the usefulness of unit tests
@@ -268,8 +313,16 @@ int CEspHttpServer::processRequest() | |||
ESPLOG(LogMin, "%s %s, from %s", method.str(), m_request->getPath(pathStr).str(), m_request->getPeer(peerStr).str()); | |||
else //user ID is in HTTP header | |||
ESPLOG(LogMin, "%s %s, from %s@%s", method.str(), m_request->getPath(pathStr).str(), userid, m_request->getPeer(peerStr).str()); | |||
|
|||
//checkUserAuth could declare nested span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably also move to the unit tests. Creating spans when we call ldap etc. is something worth recording in a jira to come back to later.
|
||
virtual void querySpanContextProperties(IProperties * contextProps) = 0; | ||
//virtual bool injectSpanContext(CHPCCHttpTextMapCarrier * carrier) = 0; | ||
virtual bool injectSpanContext(IProperties * contextProps) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name could be improved. Probably get rather than inject. What is the difference with the function above? If different I suspect the parameter names should be different, and possibly combine into a single call.
virtual void setSpanAttributes(const IProperties * attributes) = 0; | ||
virtual void addSpanEvent(const char * eventName) = 0; | ||
|
||
virtual void querySpanContextProperties(IProperties * contextProps) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistentency: Functions that start query generally return a value. get is generally used if returns a value in a supplied parameter.
|
||
const CHPCCHttpTextMapCarrier carrier(httpHeaders); | ||
auto globalPropegator = context::propagation::GlobalTextMapPropagator::GetGlobalPropagator(); | ||
opentelemetry::v1::context::Context currentContext = context::RuntimeContext::GetCurrent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling getCurrent() - I don't think that is valid.
init(); | ||
} | ||
|
||
CSpan(const char * spanName, SpanType type, const char * tracerName_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistency: 3 different ways of differentiating the parameter from the member i) diff name ii) use this for the member iii) append an underscore.
I don't really mind any of them (although everywhere else uses _ as a prefix rather than a suffix), but much better to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of the many reasons I prefer to prefix all members with m_
tracerName.set(parent->queryTraceName()); | ||
this->type = type; | ||
|
||
init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent: extra space
Type of change:
Checklist:
Smoketest:
Testing: